Push down `version` filter in the Delta `$history` table #16192

findinpath · 2023-02-20T21:32:57Z

Description

Doing the pushdown for the version column predicates helps out in avoiding to load unnecessary transaction log files from the storage.

As part of the changes done for this feature, the Delta $history table avoids materializing all the entries of the table at once by using the same strategy as #16112

Additional context and related issues

SELECT * FROM "test_table$history" WHERE version > 218;

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeHistoryTable.java

krvikash · 2023-02-24T10:12:07Z

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeHistoryTable.java

+        {
+            requireNonNull(startVersion, "identity is null");
+            requireNonNull(endVersion, "identity is null");
+            verify(startVersion.orElse(0L) <= endVersion.orElse(startVersion.orElse(0L)), "startVersion is greater than endVersion");


Looks like a duplicate code from DeltaLakeTransactionLogEntryIterator? we can extract into a method verifyVersion?

...main/java/io/trino/plugin/deltalake/transactionlog/DeltaLakeTransactionLogEntryIterator.java

findinpath · 2023-02-27T06:34:54Z

Rebase on top of master to deal with conflicting files.

homar · 2023-02-28T09:37:25Z

do you need those .crc files? Wouldn't all the tests work without them ?

findinpath · 2023-02-28T11:18:48Z

do you need those .crc files? Wouldn't all the tests work without them ?

@homar no the files are not needed for the tests to run successfully. I copied the content of the table as it was from Databricks. I see a mixture in the test resources of _delta_log directories with and without .crc files. I don't feel strongly about keeping/removing those files. If you insist, we can have them removed.

homar · 2023-02-28T11:52:57Z

do you need those .crc files? Wouldn't all the tests work without them ?

@homar no the files are not needed for the tests to run successfully. I copied the content of the table as it was from Databricks. I see a mixture in the test resources of _delta_log directories with and without .crc files. I don't feel strongly about keeping/removing those files. If you insist, we can have them removed.

I don't have strong opinion. I just don't think we need them so maybe it would be better not to have unnecessary files. But you don't have to remove them if you don't agree

homar · 2023-02-28T11:53:05Z

do you need those .crc files? Wouldn't all the tests work without them ?

@homar no the files are not needed for the tests to run successfully. I copied the content of the table as it was from Databricks. I see a mixture in the test resources of _delta_log directories with and without .crc files. I don't feel strongly about keeping/removing those files. If you insist, we can have them removed.

I don't have strong opinion. I just don't think we need them so maybe it would be better not to have unnecessary files. But you don't have to remove them if you don't agree

findinpath · 2023-03-07T10:36:21Z

Rebasing on master to deal with code conflicts.

alexjo2144

First two commits only

alexjo2144 · 2023-03-07T16:35:05Z

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeHistoryTable.java

+                stream(commitInfoEntries)
+                        .map(commitInfoEntry -> getRecord(commitInfoEntry, timeZoneKey))
+                        .iterator();


Rather than going back and forth to Stream you can just use Iterables.transform or Iterators.transform

I did change to Iterables.transform in the 1st commit.
However, in the 2nd commit, I find the stream approach more straightforward to read. I don't see how to get to a similar result without Streams.stream method.

I think you're just looking for Iterators.concat? To replace the Stream flatMap

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeHistoryTable.java

.../src/main/java/io/trino/plugin/deltalake/transactionlog/DeltaLakeTransactionLogIterator.java

alexjo2144 · 2023-03-07T16:53:31Z

.../src/main/java/io/trino/plugin/deltalake/transactionlog/DeltaLakeTransactionLogIterator.java

+import static java.util.Objects.requireNonNull;
+
+public class DeltaLakeTransactionLogIterator
+        implements Iterator<List<DeltaLakeTransactionLogEntry>>


I would prefer a flat Iterator<DeltaLakeTransactionLogEntry>

The flat iterator comes with increased overhead. I created initially such an iterator, but it has rather high complexity for little gain - the transaction log file is read at once.

See draft implementation
https://github.com/trinodb/trino/compare/f0edb4afb7f92815526e121acffbef19382a9558..78ef37c36721b29952f052fb7da9f1a5bb847bb9

alexjo2144 · 2023-03-07T16:55:59Z

.../src/main/java/io/trino/plugin/deltalake/transactionlog/DeltaLakeTransactionLogIterator.java

+        if (stopped) {
+            return Optional.empty();
+        }


stopped seems unnecessary to me, but maybe I'm missing something

without stopped we could call hasNext() several times and this would incur further unnecessary reads in case when the endVersion is not present. Same thing for the situation when a transaction log in between - e.g. 1,2,3,5,6 - note that 4 is missing - is missing.

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeHistoryTable.java

alexjo2144 · 2023-03-07T19:08:17Z

...ake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeSystemTablesWithAccessTracking.java

+                            "00000000000000000006.json", 17,
+                            "_last_checkpoint", 17));


Do you have a sense of why these two are so high on these queries?

I counted while debugging 9 times the call to io.trino.connector.system.SystemTablesMetadata#checkAndGetTable while performing a SELECT from the $history system metadata table.

Each of this calls eventually causes a subsequent DeltaLakeMetadata#getTableHandle() call which calls TransactionLogParser#tryReadLastCheckpoint.

I was planning to create an issue to further investigate whether we could spare some getTableHandle() calls.

homar · 2023-03-08T09:22:24Z

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeHistoryTable.java

+                        fileSystem,
+                        new Path(metastore.getTableLocation(tableName, session)),
+                        Optional.empty(),
+                        Optional.empty()))


Shouldn't these 4 lines be more indented ?

I haven't found a better way to highlight the stream. Could you pls suggest a change in this regard?

…table

findinpath · 2023-03-09T10:05:34Z

CI hit #15187

findinpath · 2023-07-28T14:13:32Z

Superseded by #18427

cla-bot bot added the cla-signed label Feb 20, 2023

findinpath force-pushed the findinpath/delta-history-table-version-pushdown branch 2 times, most recently from f0edb4a to 78ef37c Compare February 21, 2023 12:38

krvikash self-requested a review February 21, 2023 13:42

findinpath marked this pull request as draft February 21, 2023 23:03

findinpath force-pushed the findinpath/delta-history-table-version-pushdown branch from 78ef37c to f24e14d Compare February 21, 2023 23:29

findinpath marked this pull request as ready for review February 21, 2023 23:30

findinpath requested review from ebyhr and alexjo2144 February 21, 2023 23:30

findinpath self-assigned this Feb 21, 2023

findinpath force-pushed the findinpath/delta-history-table-version-pushdown branch 5 times, most recently from 88b453b to cf835ab Compare February 22, 2023 15:06

findinpath mentioned this pull request Feb 23, 2023

Implement table changes function for delta lake #16205

Merged

krvikash reviewed Feb 24, 2023

View reviewed changes

findinpath force-pushed the findinpath/delta-history-table-version-pushdown branch 4 times, most recently from 0323ce7 to 389fe6c Compare February 26, 2023 08:39

findinpath force-pushed the findinpath/delta-history-table-version-pushdown branch from 389fe6c to bbfbbed Compare February 27, 2023 06:35

findinpath requested a review from krvikash February 27, 2023 06:37

homar approved these changes Feb 28, 2023

View reviewed changes

findinpath force-pushed the findinpath/delta-history-table-version-pushdown branch from bbfbbed to 9bfc116 Compare February 28, 2023 19:26

github-actions bot added the delta-lake Delta Lake connector label Feb 28, 2023

findinpath force-pushed the findinpath/delta-history-table-version-pushdown branch from 9bfc116 to 49d11bd Compare March 7, 2023 10:53

alexjo2144 reviewed Mar 7, 2023

View reviewed changes

findinpath force-pushed the findinpath/delta-history-table-version-pushdown branch from 49d11bd to 903b2e3 Compare March 7, 2023 20:41

findinpath requested a review from alexjo2144 March 7, 2023 20:41

homar reviewed Mar 8, 2023

View reviewed changes

homar approved these changes Mar 8, 2023

View reviewed changes

Use cursor-based implementation for the Delta $history metadata table

33fb62f

findinpath force-pushed the findinpath/delta-history-table-version-pushdown branch from 903b2e3 to 1119dd8 Compare March 9, 2023 06:51

findinpath added 3 commits March 9, 2023 07:52

Avoid materializing at once all records in Delta $history metadata …

6d63ff6

…table

Provide injection mechanism for the file system factory

a9905fc

Push down version filter in the Delta $history metadata table

0760302

findinpath force-pushed the findinpath/delta-history-table-version-pushdown branch from 1119dd8 to 0760302 Compare March 9, 2023 06:53

kokosing force-pushed the master branch from 3f05134 to 58d6356 Compare March 14, 2023 11:34

findinpath mentioned this pull request Jul 28, 2023

Performance improvements for the Delta Lake history system table #18427

Merged

findinpath closed this Jul 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Push down `version` filter in the Delta `$history` table #16192

Push down `version` filter in the Delta `$history` table #16192

findinpath commented Feb 20, 2023

krvikash Feb 24, 2023

findinpath commented Feb 27, 2023

homar commented Feb 28, 2023

findinpath commented Feb 28, 2023

homar commented Feb 28, 2023

homar commented Feb 28, 2023

findinpath commented Mar 7, 2023

alexjo2144 left a comment

alexjo2144 Mar 7, 2023

findinpath Mar 7, 2023

alexjo2144 Mar 7, 2023

alexjo2144 Mar 7, 2023

findinpath Mar 7, 2023

alexjo2144 Mar 7, 2023

findinpath Mar 7, 2023

alexjo2144 Mar 7, 2023

findinpath Mar 7, 2023

homar Mar 8, 2023

findinpath Mar 9, 2023

findinpath commented Mar 9, 2023

findinpath commented Jul 28, 2023

Push down version filter in the Delta $history table #16192

Push down version filter in the Delta $history table #16192

Conversation

findinpath commented Feb 20, 2023

Description

Additional context and related issues

Release notes

Choose a reason for hiding this comment

findinpath commented Feb 27, 2023

homar commented Feb 28, 2023

findinpath commented Feb 28, 2023

homar commented Feb 28, 2023

homar commented Feb 28, 2023

findinpath commented Mar 7, 2023

alexjo2144 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findinpath commented Mar 9, 2023

findinpath commented Jul 28, 2023

Push down `version` filter in the Delta `$history` table #16192

Push down `version` filter in the Delta `$history` table #16192